Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nl #426

Merged
merged 38 commits into from
Jul 21, 2016
Merged

Nl #426

merged 38 commits into from
Jul 21, 2016

Conversation

puneith
Copy link
Contributor

@puneith puneith commented Jul 20, 2016

@jerjou @waprin please review.

Jerjou Cheng and others added 15 commits July 19, 2016 18:27
Change-Id: I089b244f1a0a2210fffab6f5747eed9f6150147a
Change-Id: I8ff59d08f2ae8ce4cb3dd0b57b3548db3d5b8add
Change-Id: I1367429c3ceaefd0b0d36d9d7cad6ca29cbfdd2d
Change-Id: Iecc1327db4aa21eb5ee61ce58bba3d142e600734
Change-Id: I33a6c3259dc5a9b0cc1ad081cd57acc452fc6b1a
Change-Id: Ic2b721f775601d2dfb718be26d425e7c2e91eb8a
Change-Id: I2eac2d41e3f8d96c1d049dbbc7b48c96d5fded0a
Change-Id: I026963be93ca3ef776420807e14abdcfcc5fe95f
Change-Id: Iab58378f40c760e88f2b3cc0806985894ce77e76
Change-Id: Ib2f1a2b04c0d00cd62d178d92430379b64ea9781
Change-Id: Iea8672b60bd6ebe9602378bb8b2622a44b48e608
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 20, 2016
@theacodes theacodes added In Progress and removed cla: yes This human has signed the Contributor License Agreement. labels Jul 20, 2016
items.sort(reverse=True)
items = [json.dumps(item[1]) for item in items]

if reverse_bool:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry if I'm misunderstanding this, but it looks like you are reversing then condtionally reverse again. any way to write to just conditionally reverse once?

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 20, 2016

try:
sentiments = [get_sentiment(service, sentence) for sentence in sentences]
except HttpError as e:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This try/catch is unnecessary.

@waprin
Copy link
Contributor

waprin commented Jul 21, 2016

LGTM

@waprin
Copy link
Contributor

waprin commented Jul 21, 2016

Given Traivs passes I'm sure it's fine but is this a nox issue?

import file mismatch:
imported module 'main_test' has this __file__ attribute:
  /Users/waprin/code/python-docs-samples/language/movie_nl/main_test.py
which is not the same as the test file we want to collect:
  /Users/waprin/code/python-docs-samples/language/ocr_nl/main_test.py

@waprin
Copy link
Contributor

waprin commented Jul 21, 2016

That's when I just run

nox --session tests -- language/

@puneith
Copy link
Contributor Author

puneith commented Jul 21, 2016

So when I do nox it is successful. I see there is movie_nl and ocr_nl in your output above? Is this something with ocr? Donno?

@waprin
Copy link
Contributor

waprin commented Jul 21, 2016

Are you doing just nox or the command I ran that just does the language directory? It passes on Travis so likely an issue on my laptop ,just mentioning here....

@puneith
Copy link
Contributor Author

puneith commented Jul 21, 2016

@waprin I had to update the README and also get rid of the FileType for the rank feature.

@waprin
Copy link
Contributor

waprin commented Jul 21, 2016

alright i just ran commands again and worked for me. meeeerge away

@waprin
Copy link
Contributor

waprin commented Jul 21, 2016

I still have that test issue but again pretty sure it's nox related. out of curiosity can you try that? nox --session tests -- language

@theacodes
Copy link
Contributor

You need to specify the individual sample -- language/movie_nl

On Thu, Jul 21, 2016, 3:09 PM Bill Prin notifications@github.com wrote:

I still have that test issue but again pretty sure it's nox related. out
of curiosity can you try that? nox --session tests -- language


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#426 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPUcxToC4BolGGqTVBEU94FUiM8FctDks5qX-4TgaJpZM4JRN78
.

@puneith puneith merged commit df0153e into master Jul 21, 2016
@jerjou
Copy link
Contributor

jerjou commented Jul 21, 2016

Next time you merge, can you please squash your commits?

@theacodes
Copy link
Contributor

Please let Jerjou, Bill, or Myself merge in the future, just for consistency. I'm going to manually squash these commits.

@theacodes theacodes deleted the nl branch July 21, 2016 23:01
@theacodes theacodes restored the nl branch July 21, 2016 23:10
@theacodes theacodes deleted the nl branch July 21, 2016 23:17
holtskinner added a commit that referenced this pull request Jan 3, 2023
* chore(deps): update dependency google-cloud-storage to v2.7.0

* docs(samples): removed batch_process_documents_sample_bad_input_test

Co-authored-by: Renovate Bot <bot@renovateapp.com>
holtskinner added a commit that referenced this pull request Jan 4, 2023
* chore(deps): update dependency google-cloud-storage to v2.7.0

* docs(samples): removed batch_process_documents_sample_bad_input_test

Co-authored-by: Renovate Bot <bot@renovateapp.com>
dandhlee pushed a commit that referenced this pull request Jan 5, 2023
* chore(deps): update dependency google-cloud-storage to v2.7.0

* docs(samples): removed batch_process_documents_sample_bad_input_test

Co-authored-by: Renovate Bot <bot@renovateapp.com>
Sita04 pushed a commit that referenced this pull request Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants